Run rust-fmt On Doc Comments #104058
Run rust-fmt On Doc Comments #104058
Conversation
|
r? @scottmcm (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| //! vec![Edge { node: 1, cost: 1 }, | ||
| //! Edge { node: 3, cost: 3 }, | ||
| //! Edge { node: 4, cost: 1 }], | ||
| //! vec![Edge { node: 1, cost: 1 }, Edge { node: 3, cost: 3 }, Edge { node: 4, cost: 1 }], |
There was a problem hiding this comment.
It looks like this is using rustc's rustfmt settings including use_small_heuristics = "Max". I think default settings would be more appropriate for library docs. But maybe that's getting too picky.
There was a problem hiding this comment.
It might be tricky to do that without splitting the code blocks into their own files and include them (which I'm happy to try, if that would be an acceptable solution). An alternate solution would to instead add rustfmt file(s) for the rust library directories (but then it's applied across the libraries outside of doc comments). I don't think having specific format settings for docs is something that's even planned in rustfmt.
There was a problem hiding this comment.
It might be tricky to do that without splitting the code blocks into their own files and include them (which I'm happy to try, if that would be an acceptable solution).
That would make updating documentation along with functions more tedious and easy to miss. Some macro-generated code already contains lots of annotations, an include could be easily missed.
An alternate solution would to instead add rustfmt file(s) for the rust library directories (but then it's applied across the libraries outside of doc comments).
That'd lead to reformatting all of the library code, no?
I don't think having specific format settings for docs is something that's even planned in rustfmt.
Well, if we decide that doc comment examples need different formatting then something like that might be necessary.
|
Whether we want to do this sounds like a policy decision to me, so I'm going to flip this over to a new team member: |
scottmcm
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #104418) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Whether we want to do this sounds like a policy decision to me, so I'm going to flip this over to a new team member
Oh boy. What a terrible pick. I never had a good relationship with auto-formatters.
Soo... I don't think this makes sense unless it's enforced by tidy because otherwise there'll be drift and people occasionally doing drive-by format-the-world commits which are terrible for browsing git history.
And if we do enforce it via tidy then as @camsteffen mentions we might want a different policy for how doc comments are formatted vs. how internal code is formatted since they're displayed in different places (rustdoc with its width-limited layout vs. many different source code viewers). I have managed to avoid rustfmt enough that I don't even know enough about rustfmt.toml to make specific suggestions here.
Maybe if you think there are a few especially egregious comments that need fixing that could be done manually in a more targeted manner?
I'll see if other team members have opinions
| //! format!("Hello"); // => "Hello" | ||
| //! format!("Hello, {}!", "world"); // => "Hello, world!" | ||
| //! format!("The number is {}", 1); // => "The number is 1" | ||
| //! format!("{:?}", (3, 4)); // => "(3, 4)" | ||
| //! format!("{value}", value=4); // => "4" | ||
| //! format!("Hello"); // => "Hello" | ||
| //! format!("Hello, {}!", "world"); // => "Hello, world!" | ||
| //! format!("The number is {}", 1); // => "The number is 1" | ||
| //! format!("{:?}", (3, 4)); // => "(3, 4)" | ||
| //! format!("{value}", value = 4); // => "4" | ||
| //! let people = "Rustaceans"; | ||
| //! format!("Hello {people}!"); // => "Hello Rustaceans!" | ||
| //! format!("{} {}", 1, 2); // => "1 2" | ||
| //! format!("{:04}", 42); // => "0042" with leading zeros | ||
| //! format!("{:#?}", (100, 200)); // => "( | ||
| //! // 100, | ||
| //! // 200, | ||
| //! // )" |
There was a problem hiding this comment.
This was deliberately formatted that way for readability. rustfmt has some skip annotation that could help here.
| /// #![forbid(unsafe_code)] // with exclusive accesses, | ||
| /// // `UnsafeCell` is a transparent no-op wrapper, | ||
| /// // so no need for `unsafe` here. |
There was a problem hiding this comment.
this formatting may be somewhat questionable, but the rustfmt output is no better
| //! vec![Edge { node: 1, cost: 1 }, | ||
| //! Edge { node: 3, cost: 3 }, | ||
| //! Edge { node: 4, cost: 1 }], | ||
| //! vec![Edge { node: 1, cost: 1 }, Edge { node: 3, cost: 3 }, Edge { node: 4, cost: 1 }], |
There was a problem hiding this comment.
It might be tricky to do that without splitting the code blocks into their own files and include them (which I'm happy to try, if that would be an acceptable solution).
That would make updating documentation along with functions more tedious and easy to miss. Some macro-generated code already contains lots of annotations, an include could be easily missed.
An alternate solution would to instead add rustfmt file(s) for the rust library directories (but then it's applied across the libraries outside of doc comments).
That'd lead to reformatting all of the library code, no?
I don't think having specific format settings for docs is something that's even planned in rustfmt.
Well, if we decide that doc comment examples need different formatting then something like that might be necessary.
|
Ok, so I see you were previously directed to make the PR smaller. After having discussed it with the libs team there is at least some preference to have formatting but only if this automatically enforced via tidy and can be automatically applied via I suggest to do the following
As you can see from a few of my review comments here, blindly applying auto-formatting without looking whether some manually arranged formatting should be preserved is also an issue and will need additional review, so it's unlikely that such a change will just be waved through. I'll close this issue since we can't take it as-is. |
Redo of #102285, but only for
/library